ESQL - full text functions verifier tests refactor #128775

carlosdelest · 2025-06-02T16:03:12Z

Refactors tests on VerifierTests to include all full text functions

carlosdelest · 2025-06-02T16:05:58Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

-                plan,
-                condition,
-                Term.class,
+                FullTextFunction.class,


Simplified this to just run on all FullTextFunctions

carlosdelest · 2025-06-02T16:06:30Z

...esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MultiMatch.java

        return fields.stream()
            .map(
-                (Expression field) -> isNotNull(field, sourceText(), FIRST).and(
+                (Expression field) -> isNotNull(field, sourceText(), SECOND).and(


Fixed a bug in MultiMatch when refactoring the tests

elasticsearchmachine · 2025-06-02T16:08:13Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

…tests-refactor

…unctions-verifier-tests-refactor' into non-issue/esql-full-text-functions-verifier-tests-refactor

…tests-refactor

ioanatia · 2025-06-03T09:53:27Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-                + " or first_name is not null) or (length(first_name) > 12 and match(last_name, \"Smith\"))"
-        );
-        query("from test | where " + functionInvocation + " or (last_name is not null and first_name is null)");
+        if ("KQL".equals(functionName) || "QSTR".equals(functionName)) {


does this test need to be limited to just KQL and QSTR? I'd expect the same error for match too

That's limited to QSTR / KQL, as these do not include a field. MATCH et al would fail on not having an actual field to look into, as this test case uses ROW to generate a column instead.

ioanatia · 2025-06-03T09:56:23Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-            "1:45: [QSTR] function cannot be used after RENAME",
-            error("from test | rename last_name as full_name | where qstr(\"Anna\")")
-        );
+    public void testNonFieldBasedFullTextFunctionsNotAllowedAfterCommands(String functionName, String functionInvocation) throws Exception {


should these helper methods be private?

Yes! Done in ac0982a

ioanatia · 2025-06-03T09:57:25Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-            "1:48: [MATCH] function cannot operate on [text::keyword], which is not a field from an index mapping",
-            error("row n = null | eval text = n + 5 | where match(text::keyword, \"Anna\")")
-        );
+    public void testQueryStringFunctionsNotAllowedAfterCommands() throws Exception {


can we group this one with the KQL equivalent in one function? so we keep the same pattern that we have in the rest of the file

Sure, done in ac0982a

ioanatia · 2025-06-03T10:00:00Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-        assertEquals(
-            "1:67: [MATCH] function cannot operate on [initial], which is not a field from an index mapping",
-            error("from test | eval initial = substring(first_name, 1) | where match(initial, \"A\")")
+    public void testFieldBasedWithNonIndexedColumn(String functionName, String functionInvocation, String functionType) {


I would have a different name pattern for these helper functions e.g. checkFieldBasedWithNonIndexedColumn.
we have other functions like this e.g. checkFullTextFunctionsOnlyAllowedInWhere

Good call, I have been inconsistent here. Done in ac0982a

…unctions-verifier-tests-refactor' into non-issue/esql-full-text-functions-verifier-tests-refactor

carlosdelest

Thanks for your review @ioanatia ! I've fixed the inconsistent naming you found.

carlosdelest · 2025-06-03T10:15:38Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-                + " or first_name is not null) or (length(first_name) > 12 and match(last_name, \"Smith\"))"
-        );
-        query("from test | where " + functionInvocation + " or (last_name is not null and first_name is null)");
+        if ("KQL".equals(functionName) || "QSTR".equals(functionName)) {


That's limited to QSTR / KQL, as these do not include a field. MATCH et al would fail on not having an actual field to look into, as this test case uses ROW to generate a column instead.

carlosdelest · 2025-06-03T10:17:12Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-            "1:45: [QSTR] function cannot be used after RENAME",
-            error("from test | rename last_name as full_name | where qstr(\"Anna\")")
-        );
+    public void testNonFieldBasedFullTextFunctionsNotAllowedAfterCommands(String functionName, String functionInvocation) throws Exception {


Yes! Done in ac0982a

carlosdelest · 2025-06-03T10:17:17Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-            "1:48: [MATCH] function cannot operate on [text::keyword], which is not a field from an index mapping",
-            error("row n = null | eval text = n + 5 | where match(text::keyword, \"Anna\")")
-        );
+    public void testQueryStringFunctionsNotAllowedAfterCommands() throws Exception {


Sure, done in ac0982a

carlosdelest · 2025-06-03T10:17:29Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-        assertEquals(
-            "1:67: [MATCH] function cannot operate on [initial], which is not a field from an index mapping",
-            error("from test | eval initial = substring(first_name, 1) | where match(initial, \"A\")")
+    public void testFieldBasedWithNonIndexedColumn(String functionName, String functionInvocation, String functionType) {


Good call, I have been inconsistent here. Done in ac0982a

carlosdelest · 2025-06-03T10:20:11Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/mapping-full_text_search.json

I've added this new mapping, as I found easier when working on KNN to have a separate mapping for full text functions than to modify the default mapping - which impacted a lot of other tests.

This way we can be the owners of this mapping in case we want to add later for example sparse_vector field types, and not impact other test cases.

ioanatia

looks good - left 2 nitpicky comments which can be addressed without requesting another review.
I do want to callout that this will generate merge conflicts with #127661 which should be fine, just letting @kderusso know

ioanatia · 2025-06-03T10:32:46Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-            "1:36: [MultiMatch] function is only supported in WHERE and STATS commands\n"
-                + "line 1:55: [MultiMatch] function cannot operate on [title], which is not a field from an index mapping",
-            error("row title = \"brown fox\" | eval x = multi_match(\"fox\", title)")
+    private void testFullTextFunctionsConstantQuery(String functionInvocation, String argOrdinal) throws Exception {


should be checkFullTextFunctionsConstantQuery 🙈

Right - did that on 2818298

ioanatia · 2025-06-03T10:33:11Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-        assertEquals(
-            "1:19: first argument of [multi_match(null, first_name)] cannot be null, received [null]",
-            error("from test | where multi_match(null, first_name)")
+    private void testFullTextFunctionNullArgs(String functionInvocation, String argOrdinal) throws Exception {


should be checkFullTextFunctionNullArgs

Ouch - did that on 2818298

elasticsearchmachine · 2025-06-03T13:01:30Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 128775

carlosdelest · 2025-06-03T15:02:04Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19

Questions ?

Please refer to the Backport tool documentation

(cherry picked from commit dc3d515) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MultiMatch.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

carlosdelest added 6 commits June 2, 2025 17:57

Refactor verifier tests

28c00b2

Refactor verifier tests

cd7a468

Simplify tests

c417c01

Simplify tests

ba53a4d

Simplify tests

e35cb96

Fix merge

6580832

elasticsearchmachine added the v9.1.0 label Jun 2, 2025

carlosdelest commented Jun 2, 2025

View reviewed changes

carlosdelest marked this pull request as ready for review June 2, 2025 16:07

carlosdelest requested review from ChrisHegarty, afoucret, ioanatia, svilen-mihaylov-elastic and tteofili June 2, 2025 16:07

elasticsearchmachine removed the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Jun 2, 2025

carlosdelest and others added 4 commits June 3, 2025 08:22

Merge branch 'main' into non-issue/esql-full-text-functions-verifier-…

b35e06f

…tests-refactor

Add capabilities checks

43997d8

Merge remote-tracking branch 'carlosdelest/non-issue/esql-full-text-f…

0dda9d2

…unctions-verifier-tests-refactor' into non-issue/esql-full-text-functions-verifier-tests-refactor

Merge branch 'main' into non-issue/esql-full-text-functions-verifier-…

3cfdccb

…tests-refactor

ioanatia reviewed Jun 3, 2025

View reviewed changes

Add new full text functions data set and modify VerifierTests

713de2b

carlosdelest added 5 commits June 3, 2025 12:09

Don't use data with the CSV data loader, just the mapping

1774cb0

Spotless

0742884

Fix naming and visibility of helper methods

ac0982a

Fix merge

f3da4da

Merge remote-tracking branch 'carlosdelest/non-issue/esql-full-text-f…

94bb978

…unctions-verifier-tests-refactor' into non-issue/esql-full-text-functions-verifier-tests-refactor

carlosdelest commented Jun 3, 2025

View reviewed changes

carlosdelest requested a review from ioanatia June 3, 2025 10:20

ioanatia approved these changes Jun 3, 2025

View reviewed changes

carlosdelest enabled auto-merge (squash) June 3, 2025 10:52

carlosdelest added 2 commits June 3, 2025 12:54

Fix naming of helper methods

2818298

Remove colors dataset from knn

ede0896

carlosdelest merged commit dc3d515 into elastic:main Jun 3, 2025
17 of 18 checks passed

elasticsearchmachine added the backport pending label Jun 3, 2025

carlosdelest mentioned this pull request Jun 3, 2025

[8.19] ESQL - full text functions verifier tests refactor (#128775) #128838

Merged

joshua-adams-1 pushed a commit to joshua-adams-1/elasticsearch that referenced this pull request Jun 3, 2025

ESQL - full text functions verifier tests refactor (elastic#128775)

51b6066

Samiul-TheSoccerFan pushed a commit to Samiul-TheSoccerFan/elasticsearch that referenced this pull request Jun 5, 2025

ESQL - full text functions verifier tests refactor (elastic#128775)

1006872

ESQL - full text functions verifier tests refactor #128775

ESQL - full text functions verifier tests refactor #128775

Uh oh!

Conversation

carlosdelest commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioanatia Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosdelest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioanatia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 3, 2025

💔 Backport failed

Uh oh!

carlosdelest commented Jun 3, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ioanatia Jun 3, 2025 •

edited

Loading